Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7700618) 70.24% compared to head (041b2d5) 61.89%.
Report is 11 commits behind head on master.

❗ Current head 041b2d5 differs from pull request most recent head e7ce752. Consider uploading reports for the commit e7ce752 to get more accurate results

@@            Coverage Diff             @@
##           master   #10030      +/-   ##
==========================================
- Coverage   70.24%   61.89%   -8.36%     
==========================================
  Files         219      200      -19     
  Lines       48453    42558    -5895     
  Branches    11488     9292    -2196     
==========================================
- Hits        34037    26340    -7697     
- Misses      11988    14124    +2136     
+ Partials     2428     2094     -334     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is also necessary for GResources in Rust. Thanks for adding it.

This is the documentation: https://doc.rust-lang.org/std/macro.env.html. @eli-schwartz, I think you are taking the example posted too literally. While including a generated file is a solved problem in Meson, the env!() macro is useful for other things too.

I don't think Meson should forbid someone from using a feature of their language effectively.

Including a generated file in rust is a solved problem using meson?

...

Thanks for the rust docs link, but it doesn't tell me anything I didn't know and acknowledge in my original comment. I was never in doubt that rust lets you do this, I simply challenge the notion that it's a good "compiler design" (i.e. rust was wrong to allow it) and that people should use it when it is allowed.

The original PR asserted that it is useful, but didn't say why. Now you assert that it is useful too, but also don't say why. I, meanwhile, am asking why this thing that seems to be a terrible idea is in fact not only good, but so good that we should extend the meson DSL in a way that no other compiler supports and which slows down compilation by wrapping rust compile rules in a python script for something that is, of necessity, hardcoded into meson.build and always known at buildsystem generation time.

So that source code files can process environment variables.

Environment variables. Which meson loves so much the lead developer of meson would prefer to ban them entirely.

I think it's not unreasonable of me to try to understand why this new feature is needed and what the practical use cases are.

I'm perfectly okay with meson "forbidding someone from using a feature of their language effectively" when that feature is exclusively of use to move build system logic into source code files because cargo isn't a real build system.

I mean it really is just an alternative to configure_file(). It is just crappier because the Rust community decided that Cargo is an amazing build system, which it isn't. Because Cargo is so bad, Rust has env!(). That is pretty much all there is to it.

I thought this comment was interesting:

w.r.t mention of crate subprojects, I'd consider that to be best solved by handling this inside crate conversion to meson rules, not something that requires an extension to the Meson DSL (which crate subprojects wouldn't use... right?)

Maybe the problem could be solved by using configure_file() for a .env file, and then after meson setup someone could source the env file, and then hopefully rustc just picks up the environment variables that way?

w.r.t mention of crate subprojects, I'd consider that to be best solved by handling this inside crate conversion to meson rules, not something that requires an extension to the Meson DSL (which crate subprojects wouldn't use... right?)

What are you proposing? that I write a Rust parser in Meson and process every Rust file with a heuristic to determine when we could replace it with use or mod instead?

Maybe the problem could be solved by using configure_file() for a .env file, and then after meson setup someone could source the env file, and then hopefully rustc just picks up the environment variables that way?

That requires altering the source files, which isn't allowed in a wrap.

Among other things though, bindgen recommends this as the way to handle generated sources: https://rust-lang.github.io/rust-bindgen/tutorial-4.html

This may not be the way I'd design a compiler, but it is how Rust works, and refusing to make Rust work because it's not ideal isn't practical, we might as well just remove Rust support if we're going to refuse to implement common use cases because we don't find them pretty enough.

What are you proposing? that I write a Rust parser in Meson and process every Rust file with a heuristic to determine when we could replace it with use or mod instead?

No, I just want people writing meson.build for their own rust code to do it properly. :p

Do we actually need it for wraps? I thought the idea was to inspect a crate and convert cargo's output to build.ninja rules somehow. That doesn't require having people literally add an env kwarg to an actual meson.build for the crate.

No, but it likely requires the converted crate to add the keyword arguments because the rust files in the crate use include!(env!(...)) because that's idiomatically how you include generated sources in Cargo, and thus how you generate them in Rust.

Otherwise the wrap will have to patch the rust files, which we don't allow.

Was not thinking of the wrap use case when I made my comment. Thanks for reminding me!

Again, I thought the idea for crates was not to just add meson.build patches and add a bajillion things to the wrapdb, but to actually introspect cargo's output and use it to generate build.ninja rules similar to how cmake.subproject works from a high-level perspective, no meson.build involved.

Again, I thought the idea for crates was not to just add meson.build patches and add a bajillion things to the wrapdb, but to actually introspect cargo's output and use it to generate build.ninja rules similar to how cmake.subproject works from a high-level perspective, no meson.build involved.

Those rust files may already be using env!(), which we need to support, otherwise we have to patch the .rs files. There's no other solutions than:

1. support a feature of cargo + rust, (ie, this PR) so that meson works seamlessly
2. patch Rust sources to remove any use of env!() (assuming that's always possible, since Rust doesn't have a pre-processor)

At this point I'd like to wait on this until the build.py typing stuff lands, as it will be much easier to ensure everything is correct here once that happens.

This has been updated to use the nightly --set-env option, though the fallback to use a wrapper is currently not implemented. That will need to be reimplemented

@dcbaker Wondering if we should instead pretend Rust uses -DFOO=BAR like every sane compilers, and make CompilerArgs.to_native() translate them to --env-set FOO=BAR. That would make stuff like add_project_arguments() useful for Rust as well.

@dcbaker Wondering if we should instead pretend Rust uses -DFOO=BAR like every sane compilers, and make CompilerArgs.to_native() translate them to --env-set FOO=BAR. That would make stuff like add_project_arguments() useful for Rust as well.

I don't quite understand what you are saying, but it doesn't sound great.

add_project_arguments('--set-env=Hello=World', language: 'rust')

the fallback to use a wrapper is currently not implemented

Not sure we need to implement it. We could just make env usage requiring recent rustc version. Rust devs are used to always use latest version, that's not a problem.

Yes, I think it's extremely reasonable to say that we support this but only with a recent enough rustc version that it can be done sanely.

I've objected all along to adding a python wrapper which juggles os.environ -- the --set-env argument to rustc brings this more in line with -D from C/C++, which is fantastic news and makes this sane to implement. We should leave it at that.

I don't quite understand what you are saying, but it doesn't sound great.

add_project_arguments('--set-env=Hello=World', language: 'rust')

That's fair enough. In that case we don't need env kwarg at all, we can do:

executable(
  'main'
  'main.rs',
  rust_args : ['--env-set=OUT_DIR=@0@'.format(meson.current_build_dir()],
)

EDIT: What I meant is we could also allow rust_args : ['-DOUT_DIR=@0@'.format(meson.current_build_dir()], and Meson could translate that to --env-set. Just like we translate -L to /LIBPATH for MSVC. But I agree that could be a bit crazy idea :P

I've objected all along to adding a python wrapper which juggles os.environ -- the --set-env argument to rustc brings this more in line with -D from C/C++, which is fantastic news and makes this sane to implement. We should leave it at that.

Unfortunately it's not yet stable, so we're basically telling people "you have to use an unstable feature in a nightly compiler, sorry. That's not great user experience. I was going to propose that the env wrapper be added with an explicit deprecation and remove in 1.6 or 1.7, with the argument that if it's stabilized soon, that there would be a least a few rustc versions with it stabilized and we're no longer telling people "hey this thing that you really need to do idiomatic rust (ie, crates) isn't available, sorry", but we're also being clear that this is a stop gap and we're not going to provide long term fallback support since it's slow and bad. It also doesn't change anything from a language point of view so the cost from our point of view is fairly small. I might be convinced to do away with it entirely, but that assumes that it's stabilized in the next rust release.

We certainly could drop the env keyword argument, I had mostly used it because originally this was only implemnted as a wrapper (it's obviously been in the works a long time). I think my preference would be to drop env entirely. I'm nervious about the -D thing, since what do we do if rust adds support for -D to mean somethign else in the future?

This push is ugly, it needs some patches split, doc changes, etc. However, it does away with the env keyword, and just reads the --set-env command directly in the backend to decide to use the wrapper. The result is that we have barely any code required for the wrapper (and I can probably tuck it all up in one comment for easy removal later), to get a useful feature, and it's still only used if the feature is enabled.